-
Notifications
You must be signed in to change notification settings - Fork 4
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
DM-41711: Upgrade QuantumGraphExecutionReport to handle multiple overlapping graphs #294
Conversation
705ee46
to
a6c4f8d
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #294 +/- ##
==========================================
- Coverage 89.55% 89.40% -0.15%
==========================================
Files 50 50
Lines 4317 4427 +110
Branches 693 718 +25
==========================================
+ Hits 3866 3958 +92
- Misses 325 336 +11
- Partials 126 133 +7 ☔ View full report in Codecov by Sentry. |
c4711de
to
9ea819a
Compare
9f4bd4c
to
43b712c
Compare
02adf39
to
2cb5b3a
Compare
78398d1
to
ea0b027
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mostly suggestions about comments, but some code questions. Merge approved.
help="Collection(s) associated with said graphs/processing." | ||
" For use in `lsst.daf.butler.registry.queryDatasets` if paring down the query would be useful.", | ||
) | ||
@click.option("--where", default="", help="A 'where' string to use to constrain the collections, if passed.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could reuse click options from butler. But help strings are a little different here, so not sure which is better.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can override the default help if you use a standard option.
" quanta to the screen. Note: the default is to output a yaml file with error information" | ||
" (data_ids and associated messages) to the current working directory instead.", | ||
help="Use the QuantumProvenanceGraph instead of the QuantumGraphExecutionReport, " | ||
"even when there is only one qgraph.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it use v2 automatically when more than one qgraph? If yes, maybe change help message.
|
||
REPO is the location of the butler/registry config file. | ||
|
||
QGRAPH is the URL to a serialized Quantum Graph file. | ||
QGRAPHS is a `Sequence` of links to serialized Quantum Graphs which have | ||
been executed and are to be analyzed. | ||
""" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure why it wasn't this way before, but doesn't this function need the LSST style docstring with PARAMETERS?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No because click formats this string as help text and doesn't understand numpydoc.
butler_config : `str` | ||
The Butler used for this report. This should match the Butler used | ||
for the run associated with the executed quantum graph. | ||
qgraph_uris : `Sequence[str]` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the formatting of this is different, but cannot find exact example. See https://developer.lsst.io/python/numpydoc.html#sequence-types for list example.
tests/test_cliCmdReport.py
Outdated
|
||
with open("delete_me.yaml", "w") as f: | ||
yaml.safe_dump(report_output_dict, f) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging lines accidentally left in.
… user to pass graphs in order
4f218a2
to
8e7c828
Compare
8e7c828
to
53d339a
Compare
Depends on lsst/pipe_base#426
Checklist
doc/changes